Removed wrong request_id in messages #388
Conversation
WalkthroughThe pull request introduces changes to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/app/admin_take_dispute.rs(1 hunks)src/app/fiat_sent.rs(1 hunks)
🔇 Additional comments (2)
src/app/admin_take_dispute.rs (2)
122-127: LGTM! Correct removal of request_id for buyer notification.
Setting request_id to None is appropriate here since this is a push notification to the buyer about the dispute being taken, not a response to any specific request from them.
Line range hint 129-134: LGTM! Correct and consistent handling of request_id.
Setting request_id to None is appropriate for the seller notification, maintaining consistency with the buyer notification approach.
Let's verify the consistency of this pattern across the codebase:
✅ Verification successful
Based on the results, I can now generate the final response:
✅ Verified: request_id handling is consistent across the codebase
The codebase shows a clear pattern in request_id handling:
Noneis used for notification messages (as seen in admin_take_dispute.rs for both buyer and seller)- Original
request_idis preserved for direct responses (as seen in admin_cancel.rs and admin_settle.rs)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other instances of Message::new_order to ensure consistent request_id handling
# Search for Message::new_order usage patterns
rg -A 3 'Message::new_order\('
# Search for potential request_id usage in message creation
ast-grep --pattern 'Message::new_order($request_id, $_, $_, $_)'
Length of output: 1407
| @@ -66,7 +66,7 @@ pub async fn fiat_sent_action( | |||
|
|
|||
| // We a message to the seller | |||
| send_new_order_msg( | |||
| request_id, | |||
| None, | |||
There was a problem hiding this comment.
💡 Codebase verification
Pattern of request_id handling is inconsistent across the codebase
Based on the search results, there are inconsistencies in how request_id is handled for notifications:
- In
src/app/fiat_sent.rs,request_idis correctly set toNonefor seller notifications - However, similar notification scenarios in other files handle it differently:
src/app/cancel.rs: Some notifications useNonewhile others pass the originalrequest_idsrc/app/add_invoice.rs: Buyer notifications inconsistently handlerequest_idsrc/app/release.rs: Mixed usage ofNoneand originalrequest_idfor similar notification scenarios
🔗 Analysis chain
LGTM! Correct handling of request_id for notifications.
The change to set request_id to None for the seller's notification is appropriate since the seller didn't make a request and is just being notified of the buyer's action. The original request_id is correctly preserved for the buyer's response.
Let's verify this pattern is consistently applied across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other instances where notifications might need similar request_id handling
# Look for patterns where send_new_order_msg is called with request_id
# Search for all send_new_order_msg calls to review request_id handling
rg -A 5 'send_new_order_msg\('
# Search for notification-like scenarios in message sending
ast-grep --pattern 'send_new_order_msg($$$request_id$$$)'
Length of output: 15279
There was a problem hiding this comment.
I think this is normal Rabbit! Not always the use is the same...
There was a problem hiding this comment.
@arkanoider, understood! If the varying use of request_id is intentional, then no changes are needed. Thank you for clarifying.
✏️ Learnings added
Learnt from: arkanoider
PR: MostroP2P/mostro#388
File: src/app/fiat_sent.rs:69-69
Timestamp: 2024-11-08T20:37:58.724Z
Learning: In the MostroP2P codebase, variations in `request_id` handling for notifications are acceptable and may vary depending on the specific context and requirements.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
@grunch @bilthon
Some other
request_idthat can be set tononeimo.Guys check you too please...
Summary by CodeRabbit